Line-matching rewriter#940
Conversation
amomchilov
force-pushed
the
Alex/line-matching
branch
2 times, most recently
from
9aad209 to
df5c587
Compare
amomchilov
force-pushed
the
Alex/line-matching
branch
from
df5c587 to
754641a
Compare
amomchilov
force-pushed
the
Alex/line-matching
branch
from
754641a to
273ef3e
Compare
amomchilov
changed the base branch from
graphite-base/940
to
Alex/Options-object
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
amomchilov
force-pushed
the
Alex/Options-object
branch
2 times, most recently
from
eeb32d8 to
d5692f6
Compare
amomchilov
force-pushed
the
Alex/line-matching
branch
from
273ef3e to
4dafa78
Compare
| next | ||
| end | ||
|
|
||
| rewrite_annotation(annotation, is_known: true) |
There was a problem hiding this comment.
LLM review pointed out that this is getting called per signature, so we could have a case of duplicate RBS_REWRITTEN_ANNOTATION being inserted
# @override
#: (Integer) -> void
#: (String) -> void
def foo(x); endwould be translated into
# RBS_REWRITTEN_ANNOTATION: RBS_REWRITTEN_ANNOTATION: @override| # @override | ||
| #: (RBS::Signature) -> void | ||
| def rewrite_discarded_overload(signature) | ||
| @rewriter << Source::Insert.new(signature.location.start_offset + 1, " RBS_DISCARDED_OVERLOAD") |
There was a problem hiding this comment.
Another LLM edge case find: This isn't considering multi line signatures and the RBS syntax sticks around.
#: (Integer,
#| String) -> voidgets translated into
# RBS_DISCARDED_OVERLOAD: (Integer,
#| String) -> void
amomchilov
force-pushed
the
Alex/line-matching
branch
from
4dafa78 to
846ea47
Compare
amomchilov
force-pushed
the
Alex/Options-object
branch
from
d5692f6 to
7654edc
Compare
amomchilov
force-pushed
the
Alex/line-matching
branch
from
846ea47 to
0bdd70e
Compare
| #: LineMatchedRBIFormat | ||
| attr_reader :default | ||
| end | ||
| end |
There was a problem hiding this comment.
Will this be used similar to HumanReadableRBIFormat in lib/spoom/sorbet/translate/rbs_comments_to_sorbet_sigs.rb? Curious why it's not being triggered in this PR.
| #| insert_pos: Integer, | ||
| #| sorbet_replacement: String? | ||
| #| ) -> void | ||
| def apply_class_annotation(annotation, parent_node:, insert_pos:, sorbet_replacement:) = raise |
There was a problem hiding this comment.
These are making this class harder to read but I'm not sure what we can do. One alternative I kinda like it to move these abstract methods to HumanReadableTranslator instead. Call sites in this class would then delegate to the appropriate "mode" that's being set, @translation_mode.apply_class_anotation. It wouldn't get rid of the abstract but make the intent more clear. Wdyt? Maybe as a future improvement to not hold up the PR.
| #{expected_line_matched_format} | ||
|
|
||
| Actual rewritten output: | ||
| #{rewritten_output} |
There was a problem hiding this comment.
The message talks about line-matched format but rewritten_output is human readable output at this stage.
amomchilov
force-pushed
the
Alex/line-matching
branch
from
0bdd70e to
a756398
Compare
2 participants
No description provided.